Skip to content

Fix write_dataframe_async parallel write overwriting #1310

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
cubewise-gng wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix write_dataframe_async parallel write overwriting #1310

cubewise-gng wants to merge 2 commits into from

Conversation

@cubewise-gng
Copy link
Contributor

@cubewise-gng cubewise-gng commented Nov 7, 2025

To address #1307 write_dataframe_async to aggregate dataframe before parallel processing.

@MariusWirtz
Copy link
Collaborator

MariusWirtz commented Nov 8, 2025

Would it be easier if you call the existing aggregate_duplicate_intersections function directly from the write_dataframe_async function?

@cubewise-gng
Copy link
Contributor Author

cubewise-gng commented Nov 9, 2025

In case the dataframe contains a mix of string and numeric values.
I did not want to aggregate strings by calling aggregate_duplicate_intersections directly.
Is that the right approach?

@MariusWirtz
Copy link
Collaborator

MariusWirtz commented Dec 7, 2025

In case the dataframe contains a mix of string and numeric values.
I did not want to aggregate strings by calling aggregate_duplicate_intersections directly.
Is that the right approach?

Yes. Good catch. If increment is True, we should increment the numeric values and apply last-one-wins on string values.

Changing the increment default value to True is technically a breaking change, but I would argue that this counts as a bugfix.
With increment set to False (the current behaviour), the function produces nondeterministic results when handling duplicate numeric records.

I will run all tests and merge

MariusWirtz
MariusWirtz reviewed Dec 7, 2025
View reviewed changes
TM1py/Utils/Utils.py Outdated
cellset = CaseAndSpaceInsensitiveTuplesDict(
dict(zip(df.iloc[:, :-1].itertuples(index=False, name=None), df.iloc[:, -1].values))
)
return cellset
Copy link
Collaborator

@MariusWirtz MariusWirtz Dec 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should return a dataframe.

Currently, it returns a dictionary and that breaks existing code and existing test cases.

Copy link
Contributor Author

@cubewise-gng cubewise-gng Dec 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, made the correction.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2025

Tests completed for environment: tm1-11-cloud. Check artifacts for details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MariusWirtz MariusWirtz MariusWirtz left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cubewise-gng @MariusWirtz